Deduplicate aspect classes on repeated registration#418
Conversation
AspectCollector::setAround() merges class rules with array_merge(), so registering the same aspect again appends duplicate entries. Providers re-register their aspects on every application boot, and each duplicate makes GenerateProxies re-match a longer rule list against the class map on the next boot. In a test suite that boots the application per test, boot time grows linearly with the number of tests already run. Deduplicate the merged class list so repeated registration is idempotent.
📝 WalkthroughWalkthroughAspectCollector's aspect metadata storage is consolidated from a dual container/aspectRules structure into a single ChangesAspectCollector Storage Refactor and Consumers
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR eliminates quadratic boot-time growth in test suites (and long-lived workers) by making
Confidence Score: 5/5Safe to merge — the change is self-contained, the previous divergence between the two data stores is eliminated, and deduplication is now exercised on every registration path. The previous comment identified that the else (initial registration) path did not deduplicate while the re-registration path did. The new code removes that distinction entirely — there is one unconditional path that always applies array_unique. The $container secondary store and its public get()/set()/list() methods are removed, eliminating the source of divergence. All existing callers in the framework have been migrated to getClassRules(), and a grep confirms no remaining usages of the old API. New tests cover both the first-registration and repeated-registration deduplication cases. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "Update AOP collector tests for single st..." | Re-trigger Greptile |
…uplicate-registration
AspectCollector kept class-targeting rules in two stores: the generic dot-key container and the priority-aware aspect rules array. Re-registering an aspect had to keep both stores in sync, which made duplicate class rules easy to accumulate and made the proxy generator scan the same rule repeatedly against the class map. Make the aspect rules array the single source of truth, add a typed getClassRules() reader for consumers that only need aspect => class rules, and remove the old generic get/set/list metadata surface. setAround() now merges and deduplicates class rules once while preserving priority overwrite behavior on repeated registration. Update the AOP parser, proxy manager, and proxy trait to read the typed class-rule map. Also remove the redundant getRule() re-fetch in Aspect::parseClasses() now that the passed map is already the canonical class-rule data.
InteractsWithAop manually resolves matching aspects for tests that need to run the AOP pipeline without generated proxies. That helper was still reading AspectCollector through the removed generic classes metadata store. Switch it to AspectCollector::getClassRules() so the helper follows the same single-source class-rule API as the runtime proxy path. The returned shape is unchanged for the helper: aspect class names keyed to their class-targeting rule lists.
The collector no longer exposes the old generic dot-key metadata helpers, so tests should describe the actual supported AOP registry behavior instead of the removed storage mechanism. Add coverage for duplicate class rules on both initial registration and repeated registration. Assert class-rule reads through getClassRules(), and keep existing merge, priority, forget, and flush behavior covered. Update ProxyTraitTest to register aspects through setAround() instead of seeding the collector internals directly. This exercises the real registration path while preserving the default zero-priority behavior those tests relied on.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Di/Aop/AspectCollectorTest.php (1)
28-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering priority behavior on duplicate
setAround()calls.The new dedup tests correctly verify that class arrays are deduplicated on both initial registration (lines 46-54) and repeated registration (lines 56-65). However, none of these tests assert what happens to
prioritywhensetAround()is called twice for the same aspect with differing priority values (e.g., does the second call overwrite the priority, or is the original preserved?). Since the PR's core change is around repeated registration becoming idempotent, locking in this contract with a test would help prevent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Di/Aop/AspectCollectorTest.php` around lines 28 - 65, Add a test in AspectCollectorTest that exercises duplicate setAround() calls with different priority values and asserts the expected priority behavior. Use AspectCollector::setAround(), getPriority(), and getRule() to verify whether the second registration preserves or overwrites the existing priority, alongside the existing class deduplication behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Di/Aop/AspectCollectorTest.php`:
- Around line 28-65: Add a test in AspectCollectorTest that exercises duplicate
setAround() calls with different priority values and asserts the expected
priority behavior. Use AspectCollector::setAround(), getPriority(), and
getRule() to verify whether the second registration preserves or overwrites the
existing priority, alongside the existing class deduplication behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e507cb47-bd9b-49a8-a362-2e6475ceb4d2
📒 Files selected for processing (7)
src/di/src/Aop/Aspect.phpsrc/di/src/Aop/AspectCollector.phpsrc/di/src/Aop/ProxyManager.phpsrc/di/src/Aop/ProxyTrait.phpsrc/foundation/src/Testing/Concerns/InteractsWithAop.phptests/Di/Aop/AspectCollectorTest.phptests/Di/Aop/ProxyTraitTest.php
|
@anabeto93 Thanks! |
2 participants
AspectCollector::setAround()merges class rules witharray_merge(), so registering the same aspect again appends duplicate entries. Providers re-register their aspects on every application boot, and each duplicate makesGenerateProxiesre-match a longer rule list against the class map on the next boot.In a test suite that boots the application per test this becomes quadratic: on a 458-test suite with the Sentry and Telescope Guzzle aspects registered, per-test boot grew from ~0.05s on the first test to ~1.3s by the last. With deduplication the growth is gone.
AfterEachTestSubscriberalready flushes the collector between tests; this makes the registration itself idempotent so repeated boots within one process stay bounded.Test run:
Summary by CodeRabbit
Bug Fixes
Tests